Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix #5250 : Add support for logging device DPI events #5270

Merged
merged 8 commits into from
Jan 30, 2024
Merged

Fix #5250 : Add support for logging device DPI events #5270

merged 8 commits into from
Jan 30, 2024
+111 −104

Conversation

Tejas-67
Copy link
Contributor

@Tejas-67 Tejas-67 commented Dec 12, 2023 •
edited by adhiamboperes
Loading

Explanation

Fix #5250 In the EventBundleCreator class, introduced a new item in the bundle to log the screen density of the device. Stored using the key "screen_density" and value retrieved from resources.displayMetrics.screenDensity .

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

If your PR includes UI-related changes, then:

  • Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes
  • For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see RTL guide)
  • Add a video showing the full UX flow with a screen reader enabled (see accessibility guide)
  • For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included
  • Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing

@Tejas-67 Tejas-67 requested a review from a team as a code owner December 12, 2023 06:09
@Tejas-67 Tejas-67 requested a review from BenHenning December 12, 2023 06:09
@Tejas-67
Copy link
Contributor Author

@adhiamboperes the tests are failing because of the increase in bundle size by 1 due to addition of screen density value.
modifying the expected bundle size in the tests would fix this. Should I go with this fix?

@Tejas-67
Copy link
Contributor Author

@adhiamboperes please check

@adhiamboperes
Copy link
Collaborator

@adhiamboperes the tests are failing because of the increase in bundle size by 1 due to addition of screen density value. modifying the expected bundle size in the tests would fix this. Should I go with this fix?

Hi @Tejas-67, I took a look -- yes, modifying the expected size in tests should be the correct way to address this.

@Tejas-67
Copy link
Contributor Author

@adhiamboperes please check

@Tejas-67
Copy link
Contributor Author

@adhiamboperes

@oppiabot OppiaBot
Copy link

oppiabot bot commented Jan 2, 2024

Hi @Tejas-67, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added stale Corresponds to items that haven't seen a recent update and may be automatically closed. and removed stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Jan 2, 2024
adhiamboperes
adhiamboperes requested changes Jan 3, 2024
View reviewed changes
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Tejas-67!

I have left an improvement suggestion but the overall approach is in the right direction. PTAL.

utility/src/main/java/org/oppia/android/util/logging/EventBundleCreator.kt Outdated Show resolved Hide resolved
@oppiabot OppiaBot
Copy link

oppiabot bot commented Jan 10, 2024

Hi @Tejas-67, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added stale Corresponds to items that haven't seen a recent update and may be automatically closed. and removed stale Corresponds to items that haven't seen a recent update and may be automatically closed. labels Jan 10, 2024
@adhiamboperes
Copy link
Collaborator

@Tejas-67, any update on this? Please assign this PR to me once it ios ready for a final review.

@Tejas-67 Tejas-67 removed their assignment Jan 29, 2024
@Tejas-67
Copy link
Contributor Author

hey @adhiamboperes, i have done the necessary changes. Please check.

adhiamboperes
adhiamboperes approved these changes Jan 30, 2024
View reviewed changes
Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @Tejas-67. This LGTM.

@oppiabot OppiaBot
Copy link

oppiabot bot commented Jan 30, 2024

Assigning @BenHenning for code owner reviews. Thanks!

@adhiamboperes adhiamboperes merged commit 3ced7e1 into oppia:develop Jan 30, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adhiamboperes adhiamboperes adhiamboperes approved these changes

@BenHenning BenHenning Awaiting requested review from BenHenning BenHenning is a code owner automatically assigned from oppia/android-app-infrastructure-reviewers

Assignees

@BenHenning BenHenning

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Add support for logging device DPI for events
3 participants
@Tejas-67 @adhiamboperes @BenHenning